Conversation
|
@Alrighttt @ozkanonur please review |
shamardy
left a comment
There was a problem hiding this comment.
Thank you for the fixes! First review iteration!
| } | ||
|
|
||
| pub(crate) struct NftCtx { | ||
| pub(crate) guard: Arc<AsyncMutex<()>>, |
There was a problem hiding this comment.
I think instead of a guard you should have the nft_cache_db in the Nft context https://github.com/KomodoPlatform/komodo-defi-framework/blob/806ac632a2f65ce09e2215af9dc25298458d45e2/mm2src/coins/lp_coins.rs#L2577
then it can be initialized only once when from_ctx first called and then retrieved on subsequent calls and this is done using from_ctx https://github.com/KomodoPlatform/komodo-defi-framework/blob/eaed80ef9e839e6a51812723fbc2cb66aa15d8d9/mm2src/mm2_core/src/mm_ctx.rs#L608
like how it's used in CoinsContext https://github.com/KomodoPlatform/komodo-defi-framework/blob/806ac632a2f65ce09e2215af9dc25298458d45e2/mm2src/coins/lp_coins.rs#L2562
This is not a blocker since guard works right now, but it's better code structure since now we have CoinsContext and NftCtx and nft_cache_db is in CoinsContext. I think you can remove the need to build storage in every nft function also following from the above.
There was a problem hiding this comment.
I think instead of a guard you should have the nft_cache_db in the Nft context
without mutex guard, we will have the same mm2 problem due to race condition.
{"mmrpc":"2.0","error":"DB error ErrorSaving(\"Error uploading an item: \\\"DomException { obj: Object { obj: JsValue(ConstraintError: Unable to add key to index 'chain_tx_hash_index': at least one key does not satisfy the uniqueness requirements.\\\\nundefined) } }\\\"\")","error_path":"nft.wasm_storage.object_store","error_trace":"nft:115] wasm_storage:362] object_store:42]","error_type":"DbError","error_data":"ErrorSaving(\"Error uploading an item: \\\"DomException { obj: Object { obj: JsValue(ConstraintError: Unable to add key to index 'chain_tx_hash_index': at least one key does not satisfy the uniqueness requirements.\\\\nundefined) } }\\\"\")","id":null}
PS: I also caught this problem in native target. Sql propagates the error that you are trying to add item with identical primary key.
I mean if we just move nft_cache_db to NftCtx, we will have the same situation. Instead of CoinsContext::from_ctx(ctx) we will just have NftCtx::from_ctx(&ctx) here

Also dont forget, that nft_cache_db is for wasm target, for native we use sqlite_connection from mmctx.
We need to lock the whole function before using storage and sending the moralis request. So I believe it doesn't mater, where we have nft_cache_db or build storage, we need to prevent the access to storage and sending identical moralis request before previous RPC is done.
But yeah, it is a good idea, bcz nft_cache_db should be a part of context of related feature. I didnt notice it.
I think you can remove the need to build storage in every nft function also following from the above.
yep, I have it in my plans. Actually I was trying to move the process of NftBuilder creation and NftStorage building to NftCtx as in this example, but I faced with problem during creating of Box with type impl NftListStorageOps + NftTxHistoryStorageOps. Moreover traits have type Error: NftStorageError. So I postponed this idea until the next refactoring.
There was a problem hiding this comment.
moved nft_cache_db field to struct NftCtx
…t_cache_db to NftCtx
smk762
left a comment
There was a problem hiding this comment.
Testing NFT database successful:
On first load, nft list and history empty until update_nft called.
Ddid some transfers, from external and to self, and each time no change to list/history, until update called. No errors encountered.
On self send, saw block number go up in list as expected. No errors encountered.
After logging out and then in again, previous stored list/history was visible without needing to call update.
Addresswas added in nft and transaction objects:solves notes in chore(release): v1.0.6-beta #1871
solves notes in chore(release): v1.0.6-beta #1871
guard: Arc<AsyncMutex<()>>fromstruct NftCtxto lock nft functions which uses db:solves mm2 error from update_nft request return error on the first start of browser app in incognito tab and send errors to console #1909
collectmethod:fixes runtime cursor error in wasm
Uncaught Error: closure invoked recursively or after being droppedrelated toIdbCursor::continue_andIdbIndex::open_cursor_with_range_and_directionalso error from update_nft request return error on the first start of browser app in incognito tab and send errors to console #1909